Sisc1 121 베팅시 획득 포인트, 베팅 인원수 추가#122
Hidden character warning
Conversation
Walkthrough컨트롤러의 반환 타입을 Changes
Sequence Diagram(s)sequenceDiagram
participant C as 클라이언트
participant Ctrl as BettingController
participant Svc as BettingService
participant Repo as BetRoundRepository
participant DB as Database
C->>Ctrl: GET /bet/today?scope=...
Ctrl->>Svc: getActiveRoundResponse(scope)
Svc->>Repo: findTopByScopeAndActive(...)
Repo->>DB: SELECT active bet round
DB-->>Repo: BetRound 또는 null
Repo-->>Svc: Optional<BetRound>
Svc->>Svc: BetRound -> BetRoundResponse.from()
Svc-->>Ctrl: Optional<BetRoundResponse>
alt 존재
Ctrl-->>C: 200 OK (BetRoundResponse)
else 미존재
Ctrl-->>C: 404 Not Found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 주의 검토 항목:
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java (1)
125-134: BigDecimal 계산에서 double 캐스팅 사용 – 정밀도 손실 우려
getEstimatedRewardMultiplier가return BigDecimal.valueOf((double) totalPool / optionPool);처럼
long -> double -> BigDecimal경로를 타고 있어, BigDecimal을 쓰는 이점(정밀도, 예측 가능한 스케일)이 거의 사라집니다. 향후 이 값을 기반으로 실제 보상/배당을 계산할 예정이라면 정밀도가 중요해질 가능성이 높습니다.예를 들어 다음처럼 BigDecimal 연산으로 직접 나누도록 리팩터링하는 것을 제안드립니다:
- long totalPool = upTotalPoints + downTotalPoints; - long optionPool = (option == BetOption.RISE) ? upTotalPoints : downTotalPoints; + long totalPool = upTotalPoints + downTotalPoints; + long optionPool = (option == BetOption.RISE) ? upTotalPoints : downTotalPoints; ... - // 공식: (전체 베팅 포인트 / 내 옵션 포인트) * 수수료 등 보정 - return BigDecimal.valueOf((double) totalPool / optionPool); + // 공식: (전체 베팅 포인트 / 내 옵션 포인트) * 수수료 등 보정 + BigDecimal total = BigDecimal.valueOf(totalPool); + BigDecimal optionTotal = BigDecimal.valueOf(optionPool); + // 필요한 소수점 자릿수/반올림 전략은 기획에 맞춰 조정 + return total.divide(optionTotal, 2, java.math.RoundingMode.HALF_UP);소수점 자리 수(위 예시는 2자리)와 반올림 방식은 실제 기획에 맞춰 조정하면 될 것 같습니다.
backend/src/main/java/org/sejongisc/backend/betting/service/BettingService.java (2)
143-149: 베팅 통계가 취소/실패 케이스를 어떻게 다루는지 요구사항 확인 필요현재 흐름:
stake = isFree ? 0 : stakePoints;- 바로
betRound.addBetStats(option, stake);호출- 이후 유료 베팅이면 포인트 검증·차감, 실패 시 예외 → 트랜잭션 롤백
이라서, 예외로 롤백되는 케이스에서는 통계도 같이 롤백되므로 데이터 오염은 없을 것으로 보입니다.
다만 도메인 관점에서:
cancelUserBet에서 통계를 되돌리는 로직이 없어, 통계가 "현재 유효한 베팅 수/포인트"가 아니라 라운드 동안 발생한 누적 베팅 시도(취소 포함) 기준이 됩니다.- API/기획 상 "베팅 인원 수"를 UX에서 어떻게 보여줄지(현재 참여 중인지, 한 번이라도 참여했는지)에 따라 이 동작이 맞을 수도, 아닐 수도 있습니다.
요구사항이 "현재 유효한 ACTIVE 베팅 기준" 이라면:
cancelUserBet에서BetRound에 대해 역연산(removeBetStats(option, stakePoints))을 호출하는 방식을 고려해 볼 수 있을 것 같습니다.현 기획 의도를 한 번만 더 확인해 보시면 좋겠습니다.
4-4: BetRoundResponse 매핑용 서비스 메서드 추가는 깔끔합니다
getActiveRoundResponse에서Optional.map(BetRoundResponse::from)패턴을 사용해 컨트롤러가 바로 DTO를 받을 수 있게 한 구조는 깔끔해 보입니다.
기존getActiveRound(Scope)도 남겨두어 내부 용도로 사용할 수 있어 확장성도 괜찮습니다.추가로 읽기 전용 트랜잭션이 필요하다면, 향후에는 서비스 전체 혹은 개별 메서드에
@Transactional(readOnly = true)를 붙이는 것도 고려해 볼 수 있습니다.Also applies to: 182-186
backend/src/main/java/org/sejongisc/backend/betting/dto/BetRoundResponse.java (1)
12-48:expectedUpReward/expectedDownReward의미(배당 배율 vs 포인트) 정리 제안현재 DTO 주석은
// 예상 획득 포인트 (100포인트 베팅 기준 예시)라고 되어 있지만, 실제 값은
BetRound.getEstimatedRewardMultiplier(...)의 리턴값(전체 포인트 / 옵션 포인트)이라서 **"예상 포인트"라기보다는 배당 배율(멀티플라이어)**에 가깝습니다.프론트/기획에서 기대하는 값이:
- 배당 배율이라면: 필드명/주석을
expectedUpMultiplier등으로 바꾸거나, 주석을 "예상 배당 배율 (프론트에서 기준 포인트를 곱해 사용)"처럼 명확히 해 두는 편이 좋고,- **실제 예상 포인트(예: 100포인트 기준)**라면: DTO 생성 시점에 기준 포인트를 곱해 준 값을 넣는 편이 더 직관적일 것 같습니다.
지금도 기능적으로 문제는 없지만, API 스펙 의미가 처음부터 분명해져야 이후 클라이언트 코드나 문서에서 혼동이 줄어들 것 같습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/main/java/org/sejongisc/backend/betting/controller/BettingController.java(2 hunks)backend/src/main/java/org/sejongisc/backend/betting/dto/BetRoundResponse.java(1 hunks)backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java(2 hunks)backend/src/main/java/org/sejongisc/backend/betting/service/BettingService.java(3 hunks)
🔇 Additional comments (1)
backend/src/main/java/org/sejongisc/backend/betting/controller/BettingController.java (1)
10-11: DTO 기반 응답 전환 및 404 처리 방식 적절
BetRound엔티티 대신BetRoundResponseDTO를 반환하고, 활성 라운드가 없을 때404 Not Found를 돌려주는 구조로 바뀐 점이 명확하고 REST API 관점에서도 자연스럽습니다.
Optional.map(ResponseEntity::ok).orElseGet(ResponseEntity.notFound()::build)패턴도 읽기 좋고, Swagger description의 200/404 설명과 실제 동작이 잘 일치합니다.Also applies to: 51-60
backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java
Show resolved
Hide resolved
|
|
||
| // [추가] 통계 업데이트 메서드 (서비스에서 호출) | ||
| public void addBetStats(BetOption option, int points) { | ||
| if (option == BetOption.RISE) { // UP | ||
| this.upBetCount++; | ||
| this.upTotalPoints += points; | ||
| } else if (option == BetOption.FALL) { // DOWN | ||
| this.downBetCount++; | ||
| this.downTotalPoints += points; | ||
| } | ||
| } |
There was a problem hiding this comment.
동시 베팅 시 통계(LONG/COUNT) 갱신에서 Lost Update 위험
addBetStats는 BetRound 엔티티의 필드를 단순히 ++, +=로 변경하고 있습니다. 여러 사용자가 같은 라운드에 동시에 베팅하면:
- 각 트랜잭션이 같은
BetRound스냅샷을 로드하고 - 각각 통계를 증가시킨 뒤
- 마지막 커밋이 이전 커밋의 값을 덮어써서, 일부 베팅의 카운트/포인트가 유실되는 lost update 상황이 발생할 수 있습니다.
JPA에서 별도 락/버전 필드를 두지 않은 상태에서 공유 카운터를 이렇게 업데이트하면 전형적으로 발생하는 문제라, 통계 신뢰도가 크게 떨어질 수 있습니다.
대안 예시:
- 엔티티에
@Version Long version;을 추가해 낙관적 락을 걸고,OptimisticLockException발생 시 재시도 처리, 또는 BetRoundRepository에@ModifyingJPQL/Native 쿼리로
upBetCount = upBetCount + 1, upTotalPoints = upTotalPoints + :points같은 DB 단일 UPDATE 쿼리를 사용해 원자적으로 갱신
중 하나를 고려해 주시면 좋겠습니다.
🤖 Prompt for AI Agents
In backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java
around lines 113 to 123, add protection against lost-updates caused by
concurrent ++/+= updates in addBetStats: either add an optimistic lock version
field (e.g. @Version Long version) on the BetRound entity and have callers
handle OptimisticLockException with a retry loop, or remove the in-memory
increments and implement atomic DB-side updates via a repository @Modifying
JPQL/native update (e.g. set upBetCount = upBetCount + 1, upTotalPoints =
upTotalPoints + :points) so increments occur in a single atomic SQL UPDATE;
choose one approach and update service/repository logic accordingly (add version
field and retry logic if optimistic locking; or create and call the @Modifying
update method and stop mutating entity fields in-memory).
cksdid202
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java(2 hunks)backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java(2 hunks)backend/src/main/java/org/sejongisc/backend/betting/service/BettingService.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/org/sejongisc/backend/betting/service/BettingService.java
- backend/src/main/java/org/sejongisc/backend/betting/entity/BetRound.java
🔇 Additional comments (1)
backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java (1)
24-32: 검증 완료: 모든 필드명과 트랜잭션 컨텍스트가 정상입니다.원자적 업데이트 메서드의 구현이 올바르게 되어 있습니다:
- 필드명 일치: JPQL 쿼리의 모든 필드명(
betRoundID,upBetCount,upTotalPoints,downBetCount,downTotalPoints)이 BetRound 엔티티의 실제 필드명과 정확히 일치합니다.- 트랜잭션 컨텍스트:
incrementUpStats와incrementDownStats호출이@Transactional이 적용된postUserBet메서드(라인 132-155) 내에서 이루어지므로 트랜잭션 컨텍스트가 보장됩니다.- 동시성 안전성:
@Modifying(clearAutomatically = true)를 통해 영속성 컨텍스트가 적절히 동기화됩니다.
backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java (1)
24-32: 통계 JPQL UPDATE 자체는 좋지만, 필드명/트랜잭션/clear 옵션 한 번만 점검해 주세요.
- 지금처럼
upBetCount,upTotalPoints,downBetCount,downTotalPoints를 JPQLUPDATE ... SET x = x + 1형태로 올리는 건 기존 read-modify-write 방식보다 훨씬 동시성에 안전해서 방향은 좋아 보입니다.- 다만 JPQL에서는 DB 컬럼명이 아니라 엔티티의 자바 필드명을 써야 해서,
WHERE b.betRoundID = :id부분이BetRound엔티티의 실제 PK 필드명이 맞는지 한 번만 확인해 주세요. (일반적으로betRoundId처럼Id소문자 패턴을 많이 써서 헷갈리기 쉬운 부분입니다.)@Modifying(clearAutomatically = true)는 쿼리 실행 후 영속성 컨텍스트 전체를 비우기 때문에, 같은 트랜잭션 안에서User,Bet등 다른 엔티티를 이미 로드해 둔 상태에서 이 메서드를 호출하면 예상치 못하게 전부 detach될 수 있습니다.
- 만약
postUserBet트랜잭션 안에서 이 메서드 호출 후에도 다른 엔티티를 계속 다룬다면,clearAutomatically를 빼고 대신 필요 시 재조회(findById)나EntityManager#refresh를 사용하는 방식도 고려해 볼 만합니다.- 사용자 포인트 차감, 베팅 엔티티 저장, 이 통계 UPDATE 두 개가 반드시 하나의 트랜잭션 안에서 처리되는지도 함께 확인해 주세요. 중간에 예외가 나면 일부만 반영되는 상황을 막으려면 서비스 레이어에
@Transactional이 잘 잡혀 있어야 합니다.- 선택 사항이지만, 반환 타입을
void대신int로 두고 업데이트된 row 수를 받으면, 존재하지 않는 라운드 ID에 대한 호출을 방어하는 데 도움이 될 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java(2 hunks)
🔇 Additional comments (1)
backend/src/main/java/org/sejongisc/backend/betting/repository/BetRoundRepository.java (1)
3-8: Spring Data JPA용 import 정리 잘 되었습니다.이전 리뷰에서 지적되었던 Lettuce
@Param사용 문제가org.springframework.data.repository.query.Param+@Modifying,@Query조합으로 잘 교체되었습니다. 현재 import 구성은 Spring Data JPA 기준으로 문제 없어 보입니다.
2 participants
Summary by CodeRabbit
새로운 기능
버그 수정